[GSoC 2019] RabbitMQ architecture for notification system #382

aquibbaig · 2019-06-26T08:33:19Z

Aimed at fixing #373 #374 currently. This will be a WIP for now.

imphil · 2019-06-26T09:44:25Z

Thanks @aquibbaig for reducing the scope of this PR. It looks mostly good, but let's start in even smaller steps by only doing the RabbitMQ architectural stuff for now.

Please remove the commits " create new Notification entity " and " Add Web Notifications " from the PR (use git rebase -i and remove the two commits, then force-push to this branch). Make sure you have your full work history in another branch.
This needs a rebase first on top of the current master. So please do a
git fetch origin (or whatever the remote name of the upstream repository is) and then do a git rebase origin/master (again, replace the remote name)

aquibbaig · 2019-06-26T12:29:30Z

@imphil

I think the branch is fine. The git log returns 3 commits on top of master even. Also, rebasing is not adding any new changes. We have 623 commits on master and 626 on this branch. So, it shouldn't be a problem
Are you sure I should remove the first commit? I believe the first commit is needed for the architecture interface that we are building?

imphil · 2019-06-26T12:44:38Z

It's fine indeed, no idea what problem github has here. The comment about removing all bug the rabbitmq commit still holds, however.

aquibbaig · 2019-07-02T03:49:40Z

@imphil just force pushed the branch reflecting required changes

imphil

Thanks @aquibbaig for the update to this PR. I've left a couple comments inline.

On the commit structure:

Please modify the commits to have a single commit where you add all the rabbitmq configuration/classes, and the Notification(Hydrator) class. So that's essentially combining the two commits you now have into one.
Separate the demo use case of adding a message to the queue from the remaining infrastructure. so the changes to ProjectController.php should be a separate commit.

The composer.lock file changes should not be part of this PR at all.

imphil · 2019-07-03T12:33:12Z

site/app/config/packages/old_sound_rabbit_mq.yml

@@ -19,6 +19,9 @@ old_sound_rabbit_mq:
        update_project_info:
            connection:       default
            exchange_options: {name: 'update-project-info', type: fanout}
+        send_notification:
+            connection:       default
+            exchange_options: {name: 'send-notification', type: fanout}


how about notification instead of send-notification?

imphil · 2019-07-03T12:34:57Z

site/app/config/packages/old_sound_rabbit_mq.yml

+        web_notification:
+            connection:       default
+            exchange_options: {name: 'send-notification', type: fanout}
+            queue_options:    {name: 'send-notification'}


We need a setup where we have one queue per consumer, as in this picture:

In the tutorial at https://www.rabbitmq.com/tutorials/tutorial-three-python.html they're using an empty queue name to generate two unique queue names. You're using the same queue name for both consumers. Please double-check that that leads to the right queue structure.

The message currently goes through the following areas

An Exchange first

Queue next

Both the consumers where shouldHandle() checks if it will be handled by the one it is in

But I guess we want a structure where it goes through

An Exchange first

The Queue of the specific Consumer (two queues now)

To the specific Consumer where a shouldHandle() will confirm that it is handled indeed by this Consumer

I'll be working on this today itself

@imphil @agathver
So, I have come up with two promising architecture designs. I believe we need very small attention to detail here. I found there is another way here(it may be unnecessary).

A Direct Exchange architecture

Advantages

Less message load in the buffer

A fanout Exchange architecture

Advantages

No routing key required as it broadcasts messages to both queues, simpler

No logic required beforehand to generate a routing key

We want to go for the "fanout" architecture as inserting a message should be fast (it's in the synchronous page rendering path in many cases).

imphil · 2019-07-03T13:13:07Z

site/app/config/packages/old_sound_rabbit_mq.yml

+            connection:       default
+            exchange_options: {name: 'send-notification', type: fanout}
+            queue_options:    {name: 'send-notification'}
+            callback:         email_notification_consumer


Isn't that callback the default and can be omitted?

Just checked that it expects a callback to be there else an error is thrown

imphil · 2019-07-03T13:14:32Z

site/app/config/services.yml

@@ -94,3 +97,6 @@ services:
    App\RepoCrawler\GitRepoCrawler:
        tags:
            - 'app.repo_crawler'
+
+    # Register Mgilet NotificationManager as a service to be used in WebNotificationConsumer
+    Mgilet\NotificationBundle\Manager\NotificationManager: '@mgilet.notification'


We discussed this before: This commit is titled " setup Rabbit MQ architecture with respective consumers ". the mgilet bundle is not needed here, and should not appear anywhere in this commit.

This is a mistake I overlooked accidentally

imphil · 2019-07-03T13:15:16Z

site/src/Consumer/AbstractNotificationConsumer.php

+     * AbstractNotificationConsumer constructor.
+     * @param UserManagerInterface $userManager
+     */
+    public function __construct(UserManagerInterface $userManager)


Why is the UserManagerInterface part of this interface?

imphil · 2019-07-03T13:24:53Z

site/src/Util/NotificationHydrator.php

+ *
+ * Class NotificationHydrator
+ */
+class NotificationHydrator


NotificationHydrator is a strange name. This is just a plain old data class (POD), so how about Notification?

imphil · 2019-07-03T13:25:07Z

site/src/Util/NotificationHydrator.php

+    /**
+     * @var string $link
+     */
+    protected $link;


What's that for?

Actually, Mgilet provides us with a link field by default which is nullable. So, we can either add/remove this field, later on, based on our usage

So let's do it the other way around: remove the link field in our Notification(Hydrator) class, as I don't see any use for it. If mgilet uses it that's fine, we can always set it to null.

imphil · 2019-07-03T13:25:26Z

site/src/Util/NotificationHydrator.php

+    /**
+     * @var int $userIdentifier
+     */
+    protected $userIdentifier;


Is this the sender or the recipient? And it should be of type User; you can serialize that into a user id when sending.

imphil · 2019-07-03T13:26:04Z

site/src/Util/NotificationHydrator.php

+     * @var int $userIdentifier
+     */
+    protected $userIdentifier;
+


This class is missing a bit of meta data: send timestamp, author

imphil · 2019-07-03T13:39:00Z

site/src/Service/NotificationProducerService.php

+ *
+ * @author Aquib Baig <aquibbaig97@gmail.com>
+ */
+class NotificationProducerService


This class seems to be modeled after QueueDispatcherService, which isn't actually a particularly good design and needs to be fixed now that we have more than one producer service.

Please remove this class, you can inject the auto-generated service (i.e. the one you'd pass here as $notificationProducer) directly into the class method/class that needs to produce messages. (ProjectController.php in this case). Please don't use the ProducerInterface, but the actual class -- all producers conform to this interface, and autowire cannot really figure out which service class is the right one to bind now that there are two options.

So, while publishing a message, do we need to get the producer just as
this->get('@old_sound_rabbit_mq.notification_producer')->publish($msg) ?

You can inject the NotificationProducerService instead of using $this->get()

That's better

aquibbaig · 2019-07-04T10:17:05Z

@imphil Just made the changes as discussed in yesterday's meeting. Added a few comments too, sorry for the mess the last time. I hope it's fine now. If not, please let me know

imphil

Thanks @aquibbaig. Looks pretty good now, some small fixes and we're there.

imphil · 2019-07-04T10:25:06Z

site/src/Consumer/AbstractNotificationConsumer.php

+    public function execute(AMQPMessage $msg)
+    {
+        try {
+            $notification = unserialize($msg->body);


You can make $notification a member variable ($this->notification), and remove the argument from shouldHandle and handle()

site/src/Consumer/AbstractNotificationConsumer.php

imphil · 2019-07-04T10:26:51Z

site/src/Consumer/AbstractNotificationConsumer.php

+        try {
+            $notification = unserialize($msg->body);
+
+            return $this->handle($notification);


You need to call shouldHandle() as well.

imphil · 2019-07-04T10:27:39Z

site/src/Consumer/AbstractNotificationConsumer.php

+     *
+     * @return bool
+     */
+    abstract protected function shouldHandle(Notification $notification): bool;


You can always return true in the abstract implementation, it's a sane default.

imphil · 2019-07-04T10:28:39Z

site/src/Consumer/EmailNotificationConsumer.php

+use Psr\Log\LoggerInterface;
+
+/**
+ * Class EmailNotificationConsumer


Can you make this summary line more descriptive, e.g. "Send out notifications over email"

That comment needs resolving in a updated push.

imphil · 2019-07-04T10:29:54Z

site/src/Consumer/EmailNotificationConsumer.php

+     */
+    protected function handle(Notification $notification)
+    {
+        if ($this->shouldHandle($notification)) {


This should go into the execute() function (see the comment there)

imphil · 2019-07-04T10:32:30Z

site/src/Util/Notification.php

+     *
+     * @var User $user
+     */
+    protected $user;


naming: user -> recipient

imphil · 2019-07-04T10:33:26Z

site/src/Util/Notification.php

+     *
+     * @var string author
+     */
+    protected $author;


Hm, thinking about it, in almost all cases our messages come from the "system", so there's actually no sender/author. You can remove this field.

imphil · 2019-07-04T10:33:48Z

site/src/Util/Notification.php

+
+    /**
+     * Notification constructor.
+     * @throws \Exception


https://php.net/manual/en/datetime.construct.php
It emits an exception in case of error, if it contains an invalid format, but we sure don't need to throw it. As we do not set datetimes explicitely

site/app/config/packages/old_sound_rabbit_mq.yml

imphil · 2019-07-04T16:48:18Z

site/src/Consumer/AbstractNotificationConsumer.php

+     * @param LoggerInterface $logger
+     * @param Notification    $notification
+     */
+    public function __construct(LoggerInterface $logger, Notification $notification)


Please remove Notification from the constructor, it's not injected or passed there somehow: it's set in the execute() function.

imphil · 2019-07-04T16:49:20Z

site/src/Consumer/EmailNotificationConsumer.php

+class EmailNotificationConsumer extends AbstractNotificationConsumer
+{
+    /**
+     * EmailNotificationConsumer constructor.


Add a empty line after this one.

imphil · 2019-07-04T16:50:02Z

site/src/Consumer/EmailNotificationConsumer.php

+use Psr\Log\LoggerInterface;
+
+/**
+ * Class EmailNotificationConsumer


That comment needs resolving in a updated push.

imphil · 2019-07-04T16:50:32Z

site/src/Util/Notification.php

+    protected $message;
+
+    /**
+     * User associated with the Notification


Please change this comment to match the variable (it's the recipient of the notification)

imphil · 2019-07-04T16:50:52Z

site/src/Util/Notification.php

+    protected $subject;
+
+    /**
+     * Actual definition of the Notification


Actual definition -> Notification message

imphil · 2019-07-04T16:51:21Z

site/src/Util/Notification.php

+    }
+
+    /**
+     * Returns User associated with a Notification


"Get the recipient of this notification"

imphil · 2019-07-04T16:51:40Z

site/src/Util/Notification.php

+    }
+
+    /**
+     * Used to bind users to any Notification


"Set the recipient of this message"

imphil · 2019-07-04T16:51:53Z

site/src/Util/Notification.php

+    }
+
+    /**
+     * Gets the timestamp of a Notification


"Get the creation date of this notification"

imphil · 2019-07-04T16:52:12Z

site/src/Util/Notification.php

+    }
+
+    /**
+     * Sets the timestamp of a Notification


do something equivalent to the get... description for this field.

@imphil Yes. Just did that

Currently creates two consumers for email and web notifications

imphil · 2019-07-05T08:20:28Z

Thanks @aquibbaig, this now looks good! Merged & ready for new things!

aquibbaig added the gsoc Google Summer of Code label Jun 26, 2019

aquibbaig force-pushed the notifications branch 4 times, most recently from a9ffb3b to 6fb1420 Compare July 2, 2019 03:48

aquibbaig requested review from recrsn and imphil July 2, 2019 13:49

aquibbaig force-pushed the notifications branch from 6fb1420 to b5697fc Compare July 2, 2019 14:34

imphil suggested changes Jul 3, 2019

View reviewed changes

aquibbaig force-pushed the notifications branch 7 times, most recently from cb7c179 to 63a3fca Compare July 4, 2019 10:15

imphil suggested changes Jul 4, 2019

View reviewed changes

aquibbaig force-pushed the notifications branch from 63a3fca to 30d8eff Compare July 4, 2019 16:37

imphil reviewed Jul 4, 2019

View reviewed changes

create Notification data class, setup Rabbit MQ architecture

746808c

Currently creates two consumers for email and web notifications

aquibbaig force-pushed the notifications branch from 30d8eff to 746808c Compare July 4, 2019 17:10

imphil approved these changes Jul 5, 2019

View reviewed changes

imphil marked this pull request as ready for review July 5, 2019 08:19

imphil merged commit c063c54 into librecores:master Jul 5, 2019

aquibbaig changed the title ~~[GSoC 2019] Implementation of a Notification System for Librecores~~ [GSoC 2019] RabbitMQ architecture for notification system Jul 5, 2019

[GSoC 2019] RabbitMQ architecture for notification system #382

[GSoC 2019] RabbitMQ architecture for notification system #382

Conversation

aquibbaig commented Jun 26, 2019

imphil commented Jun 26, 2019

aquibbaig commented Jun 26, 2019 • edited

imphil commented Jun 26, 2019

aquibbaig commented Jul 2, 2019 • edited

imphil left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aquibbaig Jul 3, 2019 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aquibbaig Jul 3, 2019 • edited

Choose a reason for hiding this comment

A Direct Exchange architecture

A fanout Exchange architecture

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aquibbaig Jul 4, 2019 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aquibbaig commented Jul 4, 2019

imphil left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aquibbaig Jul 4, 2019 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

imphil commented Jul 5, 2019

aquibbaig commented Jun 26, 2019 •

edited

aquibbaig commented Jul 2, 2019 •

edited

aquibbaig Jul 3, 2019 •

edited

aquibbaig Jul 3, 2019 •

edited

aquibbaig Jul 4, 2019 •

edited

aquibbaig Jul 4, 2019 •

edited